Refine auto-loading behaviour #1915

jennybc · 2024-05-31T19:54:30Z

Tweak the use of RStudio's session init hook to deliver the intent of PR fix issue with autoloader in RStudio #1652.
Otherwise, if RStudio and its session init hook aren't available, just emit a message if we discover the need for renv::restore() during auto-loading.

To do/discuss:

~~After "Installing cli ..." it's easy to think the session is busy or hung. But it's not. If you hit return, you get the normal R prompt. Why does that happen?~~ As I play around with renv, I run into this phenomenon elsewhere. It feels like one or more of the messaging helpers, such as catf() or writef() sometimes fail to get their trailing newline? In any case, I think it's unrelated to what I'm doing here and out of scope for this PR.
The "Operation canceled." is unsightly and confusing. Seems like that should be suppressed or, at least in this context, hidden.
Try this on: Never try to interact with the user when auto-loading, since user interaction is UB during R startup. If we can defer loading using a hook (like rstudio.sessionInit below), do that. If we can't, emit a message telling the user what to do. ~~If possible, style the renv::restore() part as a runnable link.~~ I had to give up on that part.

jennybc · 2024-05-31T19:56:22Z

Context:

I'm launching an renv-using project, in RStudio, but I just got it from my colleague, e.g. I just git cloned it or similar. So it's clear we're using renv, but the project-specific library doesn't exist yet. While auto-loading, renv wants to interact with me and offer to do renv::restore(), but this happens during R startup, because it's triggered by code in .Rprofile and user interaction is basically undefined behaviour in this context. See this bit in the Note: section:

It is not intended that there be interaction with the user during startup code. Attempting to do so can crash the R process.

https://stat.ethz.ch/R-manual/R-patched/library/base/html/Startup.html

To this end, renv has arrangements to defer loading and any associated user interaction until R is fully initialized, using a hook (added in #1652). But I think the hook isn't currently working as intended.

Before this PR, in RStudio Version 2024.04.2+764, having done renv::init() with renv 1.0.7, I see:

# Bootstrapping renv 1.0.7 ---------------------------------------------------
- Downloading renv ... OK
- Installing renv  ... OK

- Project '~/rrr/renv-coldstart' loaded. [renv 1.0.7]
- None of the packages recorded in the lockfile are currently installed.

Note there is no offer to run renv::restore() (or even specific advice to do so).

After this PR, in RStudio, the interactive nudge to renv::restore() is deferred:

- Project '~/rrr/renv-coldstart' loaded. [renv 1.0.7.9000]
- None of the packages recorded in the lockfile are currently installed.
Would you like to run `renv::restore()` to restore the project library? [y/N]: y

The following package(s) will be updated:

...

After this PR, not in RStudio, there's no attempt to interact but the exact command is given (renv::restore()):

- Project '~/rrr/renv-coldstart' loaded. [renv 1.0.7.9000]
- None of the packages recorded in the lockfile are currently installed.
- Use `renv::restore()` to restore the project library.

jennybc · 2024-05-31T19:59:51Z

R/load.R

  # if we're running within RStudio at this point, and we're running
  # within the auto-loader, we need to defer execution here so that
  # the console is able to properly receive user input and update
  # https://github.com/rstudio/renv/issues/1650
  autoloading <- getOption("renv.autoloader.running", default = FALSE)
  if (autoloading && renv_rstudio_available()) {
-    setHook("rstudio.sessionInit", function() { renv::load(project) })
+    setHook("rstudio.sessionInit", function(...) { renv::load(project) })


Before, renv::load(project) was definitely being appended to the "rstudio.sessionInit" hook. I can see that with getHook("rstudio.sessionInit"). However it wasn't actually being executed and I think it must have failed some pre-check re: its signature. In my hands, adding ... makes this feature start to work as intended.

I think this is the explanation:

https://github.com/rstudio/rstudio/blob/8ebe5431ca2fb3c9719b70d4fbc45201d226c79c/src/cpp/session/modules/SessionRHooks.R#L26-L29

RStudio forwards arguments and the whole thing happens inside tryCatch(). So I guess if the hook function does not accept any arguments, the attempt to run the hook fails. Silently.

The whole chain is:

https://github.com/rstudio/rstudio/blob/8ebe5431ca2fb3c9719b70d4fbc45201d226c79c/src/cpp/session/modules/SessionRHooks.hpp#L19

https://github.com/rstudio/rstudio/blob/8ebe5431ca2fb3c9719b70d4fbc45201d226c79c/src/cpp/session/SessionMain.cpp#L857

https://github.com/rstudio/rstudio/blob/8ebe5431ca2fb3c9719b70d4fbc45201d226c79c/src/cpp/session/modules/SessionRHooks.hpp#L36-L39/./

https://github.com/rstudio/rstudio/blob/8ebe5431ca2fb3c9719b70d4fbc45201d226c79c/src/cpp/session/modules/SessionRHooks.R#L16-L31

R/load.R

jennybc · 2024-05-31T20:06:41Z

R/load.R

@@ -881,7 +881,7 @@ renv_load_report_synchronized <- function(project = NULL, lockfile = NULL) {
      return(FALSE)
    }

-    response <- ask("Would you like to restore the project library?", default = FALSE)
+    response <- ask("Would you like to run `renv::restore()` to restore the project library?", default = FALSE)


It feels like this could ease decision-making for the user, i.e. it's easier to answer this question confidently if you know exactly what's being proposed. Also might have some value in terms of user education.

R/load.R

jennybc · 2024-06-07T20:44:51Z

R/rstudio.R

-renv_rstudio_autoloading <- function() {
-  renv_rstudio_available() && getOption("renv.autoloader.running", default = FALSE)
-}
-


I removed the sole usage of this function.

jennybc · 2024-06-07T20:45:51Z

R/utils.R

+  autoloading <- getOption("renv.autoloader.running", default = FALSE)
+  if (autoloading)
+    return(default)


More simplification now that we've decided to never attempt interaction when autoloading, regardless of the frontend.

R/load.R

jennybc · 2024-06-24T22:50:16Z

R/load.R

@@ -70,7 +70,8 @@ load <- function(project = NULL, quiet = FALSE, profile = NULL, ...) {

  action <- renv_load_action(project)
  if (action[[1L]] == "cancel") {
-    cancel()
+    autoloading <- getOption("renv.autoloader.running", default = FALSE)
+    cancel(verbose = !autoloading)


Allows us to see the "- Operation canceled." message after overt cancelation by the user, but suppresses such a message when deferring the project load to the session init hook. Discussed elsewhere.

jennybc · 2024-06-25T00:09:19Z

This is ready now.

I took a look at adding a test but TBH it looks pretty 😬 . I think this behaviour is legitimately hard to test (interactive usage, with and without RStudio), but I'm also not familiar with all of renv's test helpers, which are extensive. So either this can go forward without an explicit test or else I could use some guidance on how to approach that.

jennybc · 2024-06-26T17:43:27Z

If you're interested in getting clickable hyperlinks w/o taking a true cli dependency (I understand why that's a nonstarter), this would be one way:

https://github.com/r-lib/rlang/blob/main/R/standalone-cli.R

I could open a separate issue for that.

kevinushey

LGTM!

kevinushey · 2024-07-02T03:56:20Z

Do we need a NEWS update? (I'm going to merge this now for testing but we can merge a separate NEWS update after if appropriate)

kevinushey · 2024-07-03T03:25:37Z

If you're interested in getting clickable hyperlinks w/o taking a true cli dependency (I understand why that's a nonstarter), this would be one way:

https://github.com/r-lib/rlang/blob/main/R/standalone-cli.R

I could open a separate issue for that.

I'd definitely be on board for this.

jennybc added 3 commits May 29, 2024 16:03

Re-order these checks

beb8d61

Put some DOTS on it

2fb9e65

Be precise about the recommended action

f24aba7

jennybc commented May 31, 2024

View reviewed changes

This was referenced Jun 3, 2024

Project Wizard: create renv::init command posit-dev/positron#3127

Closed

ark: readline() at startup causes unstability posit-dev/positron#2070

Closed

Don't message about cancellation

9d8635f

jennybc commented Jun 4, 2024

View reviewed changes

R/load.R Outdated Show resolved Hide resolved

jennybc commented Jun 4, 2024

View reviewed changes

R/load.R Show resolved Hide resolved

timtmok mentioned this pull request Jun 7, 2024

Change required renv version to 1.0.8(?) posit-dev/positron#3439

Open

jennybc mentioned this pull request Jun 7, 2024

ark: Support a session init hook posit-dev/positron#3371

Open

jennybc added 2 commits June 7, 2024 12:54

Refine action around renv::restore() during autoloading

1989c79

Make renv::restore() (maybe) a runnable hyperlink if cli is available

a11676f

jennybc commented Jun 7, 2024

View reviewed changes

R/load.R Outdated Show resolved Hide resolved

jennybc commented Jun 7, 2024

View reviewed changes

kevinushey reviewed Jun 7, 2024

View reviewed changes

R/load.R Outdated Show resolved Hide resolved

Introduce cancel(verbose =)

ec7bf57

jennybc commented Jun 24, 2024

View reviewed changes

Back out of the cli experiment

f23c248

jennybc changed the title ~~Refactor some checks when loading~~ Refine auto-loading behaviour Jun 25, 2024

jennybc marked this pull request as ready for review June 25, 2024 00:07

jennybc requested a review from kevinushey June 25, 2024 00:07

kevinushey approved these changes Jul 2, 2024

View reviewed changes

kevinushey merged commit 65ac9cb into main Jul 2, 2024
11 checks passed

jennybc mentioned this pull request Jul 3, 2024

Use standalone cli #1939

Open

jennybc deleted the bugfix/refactor-load-checks branch July 3, 2024 17:40

jabenninghoff mentioned this pull request Sep 21, 2024

renv/activate.R 1.0.8 stops processing of .Rprofile in RStudio #1990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine auto-loading behaviour #1915

Refine auto-loading behaviour #1915

jennybc commented May 31, 2024 •

edited

Loading

jennybc commented May 31, 2024 •

edited

Loading

jennybc May 31, 2024 •

edited

Loading

jennybc May 31, 2024 •

edited

Loading

jennybc May 31, 2024

jennybc May 31, 2024

jennybc Jun 7, 2024

jennybc Jun 7, 2024

jennybc Jun 24, 2024 •

edited

Loading

jennybc commented Jun 25, 2024 •

edited

Loading

jennybc commented Jun 26, 2024

kevinushey left a comment

kevinushey commented Jul 2, 2024

kevinushey commented Jul 3, 2024

Refine auto-loading behaviour #1915

Refine auto-loading behaviour #1915

Conversation

jennybc commented May 31, 2024 • edited Loading

jennybc commented May 31, 2024 • edited Loading

jennybc May 31, 2024 • edited Loading

Choose a reason for hiding this comment

jennybc May 31, 2024 • edited Loading

Choose a reason for hiding this comment

jennybc May 31, 2024

Choose a reason for hiding this comment

jennybc May 31, 2024

Choose a reason for hiding this comment

jennybc Jun 7, 2024

Choose a reason for hiding this comment

jennybc Jun 7, 2024

Choose a reason for hiding this comment

jennybc Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

jennybc commented Jun 25, 2024 • edited Loading

jennybc commented Jun 26, 2024

kevinushey left a comment

Choose a reason for hiding this comment

kevinushey commented Jul 2, 2024

kevinushey commented Jul 3, 2024

jennybc commented May 31, 2024 •

edited

Loading

jennybc commented May 31, 2024 •

edited

Loading

jennybc May 31, 2024 •

edited

Loading

jennybc May 31, 2024 •

edited

Loading

jennybc Jun 24, 2024 •

edited

Loading

jennybc commented Jun 25, 2024 •

edited

Loading